Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add stderr method #441

Merged
merged 9 commits into from
Jul 15, 2022
Merged

feat: add stderr method #441

merged 9 commits into from
Jul 15, 2022

Conversation

mdonnalley
Copy link
Contributor

Adds stderr method to Command for logging to stderr

Fixes #392

@mdonnalley mdonnalley self-assigned this Jul 14, 2022
src/command.ts Outdated
@@ -199,14 +199,24 @@ export default abstract class Command {
return Errors.error(input, options as any)
}

log(message = '', ...args: any[]): void {
stdout(message = '', ...args: any[]): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just keep this as log. I don't want to confuse anyone by using the name stdout since that's already a well known method in a node process and this method doesn't behave exactly like it.

src/command.ts Outdated
message = typeof message === 'string' ? message : inspect(message)
process.stdout.write(format(message, ...args) + '\n')
}
}

stderr(message = '', ...args: any[]): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly to my comment above I'd rather not name this stderr. Perhaps we can go with use error() or logError()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I debated the name of this... Problem is that error is already taken by another method that actually throws an error and exits the process. And then logError sorta assumes that you're only ever logging errors to stderr, which isn't a great assumption. The person asking for this feature, for example, wants to send debug logs to stderr.

logToStderr?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think using logError() and the implicit assumption of only logging errors to stderr is necessarily a bad thing. It's the same assumption the POSIX standard makes by naming that stream stderr instead of nonstdout or something that encompasses more than just errors.

If users want to use stderr for something other than errors that's up to them.

That being said, I'm fine with using logToStderr if you'd rather do that since my whole point here is just to make it clear this method is something different than process.stderr and logToStderr achieves that.

@mdonnalley mdonnalley requested a review from RodEsp July 15, 2022 19:57
@RodEsp RodEsp merged commit d9490f7 into main Jul 15, 2022
@RodEsp RodEsp deleted the mdonnalley/stderr branch July 15, 2022 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support logging to stderr
2 participants